Test and and fix detection of ci regressions of independent ports #1830
Conversation
dg0yt
force-pushed
the
ci-independent-regression
branch
from
eb444f7 to
523a634
Compare
dg0yt
changed the title
|
This reduces "independent" to "the ABI hash changed." Which is going to be true for cases with dependent feature changes as originally discussed in #794 but is also going to be true for every port that is edited in a PR. I think this change merely replaces the existing regression output with saying they are all independent. It's true that you have a test counterexample but the test misses that all PR builds for us build with if ($Output.Contains("REGRESSION: Independent regression:${Triplet} failed with BUILD_FAILED.")) {
throw 'regression (port) build failure must not be reported as independent regression'
}block is testing something that we never do? |
| View<std::string> parent_hashes) | ||
| { | ||
| // With parent hashes, ports are merely "auto selected" unless the abi hash changed. | ||
| auto const default_request_type = parent_hashes.empty() ? RequestType::USER_REQUESTED : RequestType::AUTO_SELECTED; |
There was a problem hiding this comment.
I'm also really not a fan of trying to overload RequestType like this because that controls a bunch of other settings, like whether default features are added:
vcpkg-tool/src/vcpkg/dependencies.cpp
Lines 269 to 272 in d5c3552
as well as --head/--editable handling.
(I know that once we have an ActionPlan we are out of dependencies.cpp but that this is already being used to track other things strongly suggests that it is not the right tool to track what you're trying to track here. If all we care about is "is the abi hash in parent_hashes" let's ask that question directly at reporting time rather than trying to smuggle it through this setting)
There was a problem hiding this comment.
I'm also really not a fan of trying to overload
RequestTypelike this because that controls a bunch of other settings, like whether default features are added.
With parent hashes, we do know that ports are installed for different reasons. Tracking the reason in RequestType seems a natural fit, not smuggling. AUTO_SELECTED seems to perfectly explain why an independent port is part of the installation plan.
I see that it might make sense to use the information already in the construction of the installation plan (i.e. "once, early").
as well as
--head/--editablehandling.
Not relevant for ci command.
If all we care about is "is the abi hash in parent_hashes" let's ask that question directly at reporting time rather than trying to smuggle it through this setting.
So you suggest to process the parent hashes a second time in the build result reporting (i.e "late")?
dg0yt
force-pushed
the
ci-independent-regression
branch
from
50ef2f6 to
0d09b2c
Compare
Actually this maps "independent" to "the ABI hash DID NOT change." Dependencies become visible by propagation of ABI hash changes. The cone of destruction is formed by the destruction of original ABI hashes.
|
Please forgive my inversion.
But the ABI hashes of the dependencies change in those cases too. From #794
|
|
I suppose given my inversion what you want is to not emit the "add =fail" for A in that example because it isn't edited. But I still don't think that addresses "but I didn't even edit B" |
|
Okay, I get the point. The configuration changes for the independent port(s) take them out of the overlap of (parent vs. current) ABI hash information, coloring the relevant ports as "not independent". This kills this approach. FTR for feature testing, vcpkg CI already collects the list of changed ports. This information could be an alternative starting point to color the cone of destruction. Not sure if it is not worth continuing. These regression do occur, but it is not very often now. |
Yes, now that we have the "--for-merge-with" tech that would be way better for something like this. What do you think of this? 
|
2 participants
Restore
as originally contributed in #794.